chore: add precheck for receivers account in sendRawTransactionCheck #3310

nadezhdapopovaa · 2024-12-05T12:30:50Z

Description:
Added precheck for receiver's account in sendRawTransactionCheck

github-actions · 2024-12-05T12:55:48Z

Test Results

22 files + 2 306 suites +31 58m 58s ⏱️ + 15m 20s
614 tests + 1 604 ✅ - 1 4 💤 ±0 6 ❌ +2
802 runs +107 790 ✅ +103 6 💤 +2 6 ❌ +2

For more details on these failures, see this check.

Results for commit 3b85b1a. ± Comparison against base commit b5e747f.

This pull request removes 3 and adds 4 tests. Note that renamed tests count towards both.

"after each" hook for "@release captures transfer events" ‑ RPC Server Acceptance Tests Acceptance tests @web-socket-batch-3 eth_subscribe "after each" hook for "@release captures transfer events"
"before all" hook for "Subscribes for debug" ‑ RPC Server Acceptance Tests Acceptance tests @web-socket-batch-3 eth_subscribe Subscribes to log events "before all" hook for "Subscribes for debug"
"before each" hook for "@release captures transfer events" ‑ RPC Server Acceptance Tests Acceptance tests @web-socket-batch-3 eth_subscribe "before each" hook for "@release captures transfer events"

"before all" hook for "should execute "eth_getCode" for hts token" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests eth_getCode "before all" hook for "should execute "eth_getCode" for hts token"
"before each" hook for "should execute "eth_getStorageAt" request to get current state changes" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests "before each" hook for "should execute "eth_getStorageAt" request to get current state changes"
"before each" hook: reducing balance for "reverts" ‑ RPC Server Acceptance Tests Acceptance tests @erc20 Acceptance Tests HTS token should behave like erc20 transfer from when the token owner is not the zero address when the recipient is not the zero address when the spender does not have enough allowance when the token owner does not have enough balance "before each" hook: reducing balance for "reverts"
should fail "eth_sendRawTransaction" if receiver's account has receiver_sig_required enabled ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-1 RPC Server Acceptance Tests RPC Server Acceptance Tests Transaction related RPC Calls Prechecks should fail "eth_sendRawTransaction" if receiver's account has receiver_sig_required enabled

♻️ This comment has been updated with latest results.

codecov · 2024-12-05T16:02:49Z

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.96%. Comparing base (1551da6) to head (92c28db).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/relay/src/lib/precheck.ts	75.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3310      +/-   ##
==========================================
+ Coverage   77.96%   84.96%   +6.99%     
==========================================
  Files          66       69       +3     
  Lines        4475     4641     +166     
  Branches     1003     1043      +40     
==========================================
+ Hits         3489     3943     +454     
+ Misses        612      394     -218     
+ Partials      374      304      -70

Flag	Coverage Δ
config-service	`98.14% <ø> (ø)`
relay	`78.67% <62.50%> (-0.06%)`	⬇️
server	`83.28% <ø> (ø)`
ws-server	`36.66% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
packages/relay/src/lib/precheck.ts	`87.38% <75.00%> (+13.80%)`	⬆️

... and 26 files with indirect coverage changes

quiet-node

Great work! Some blocking requests

quiet-node · 2024-12-05T16:29:42Z

packages/relay/src/lib/precheck.ts

+    if (parsedTx.to) {
+      await this.receiverAccount(parsedTx, requestDetails);
+    }


Let's move the parsedTx.to check inside the receiverAccount() precheck instead.

quiet-node · 2024-12-05T16:31:09Z

packages/relay/src/lib/precheck.ts

+   * @param {RequestDetails} requestDetails - The request details for logging and tracking.
+   */
+  async receiverAccount(tx: Transaction, requestDetails: RequestDetails) {
+    const verifyAccount = await this.mirrorNodeClient.getAccount(tx.to!, requestDetails);


Let's add the parsedTx.to check here. We can make it only run this precheck of parsedTx.to is found so in line 390 we don't have to use the ! operator.

quiet-node · 2024-12-05T16:32:48Z

packages/relay/src/lib/precheck.ts

+    const verifyAccount = await this.mirrorNodeClient.getAccount(tx.to!, requestDetails);
+
+    if (verifyAccount !== null && verifyAccount.receiver_sig_required === true) {
+      throw predefined.INTERNAL_ERROR("Receiver's signature is required.");


Hmm not sure if INTERNAL_ERROR is appropriate here. Looks like other prechecks have their own dedicated predefined errors. Should we follow the pattern and create a dedicated prefined error for receiver account?

yeah, there may be an existing error that could be used but if not add a new one

quiet-node · 2024-12-05T16:34:03Z

packages/relay/src/lib/precheck.ts

+  async receiverAccount(tx: Transaction, requestDetails: RequestDetails) {
+    const verifyAccount = await this.mirrorNodeClient.getAccount(tx.to!, requestDetails);
+
+    if (verifyAccount !== null && verifyAccount.receiver_sig_required === true) {


Could you kindly did some extra research and add a comment here stating why we want to make sure receiver_sig_required attribute is turned off?

quiet-node · 2024-12-05T16:35:27Z

packages/relay/src/lib/precheck.ts

+   * @param {Transaction} tx - The transaction.
+   * @param {RequestDetails} requestDetails - The request details for logging and tracking.
+   */
+  async receiverAccount(tx: Transaction, requestDetails: RequestDetails) {


Please add test coverage for this method in the precheck.spec.ts

quiet-node · 2024-12-05T17:11:31Z

packages/relay/src/lib/precheck.ts

+
+    if (verifyAccount !== null && verifyAccount.receiver_sig_required === true) {
+      throw predefined.INTERNAL_ERROR("Receiver's signature is required.");
+    }


Let's add another check to see if verifyAccount is null then throw a RESOURCE_NOT_FOUND and include the value of tx.to.

throw predefined.RESOURCE_NOT_FOUND(`Cannot find receiver account: tx.to=${tx.to}`);

Careful on this one since there are valid cases for tx.to not being set e.g. contract create.

Ahh yeah you're right tx.to is nullable if the transaction is a contract create transaction. Then maybe in the test make the tx.to a non-null address but mock not found. @nadezhdapopovaa please let me know if this is clear enough

Oh wait this block only executes when tx.to is not null; therefore, it is still possible for the mirror node to return a null result if tx.to is not found on the network. I'd suggest instead of checking if (verifyAccount !== null && verifyAccount.receiver_sig_required === true) for the sig required, we can pull the verifyAccount !== null out and add a new block to check the validity of the verifyAccount. If it's null then throw RESOURCE_NOT_FOUND like how we did with the tx.from.

Signed-off-by: Nadezhda Popova <[email protected]>

…nCheck Signed-off-by: Nadezhda Popova <[email protected]>

…nsactionCheck Signed-off-by: Nadezhda Popova <[email protected]>

Signed-off-by: Nadezhda Popova <[email protected]>

sonarcloud · 2024-12-11T14:54:26Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

quiet-node

Great work! However, additional tests are still needed. Also a batch of unit tests is required in the precheck.spec.ts file to functionally test the method.

quiet-node · 2024-12-11T15:23:45Z

packages/relay/src/lib/precheck.ts

+
+    if (verifyAccount !== null && verifyAccount.receiver_sig_required === true) {
+      throw predefined.INTERNAL_ERROR("Receiver's signature is required.");
+    }


Oh wait this block only executes when tx.to is not null; therefore, it is still possible for the mirror node to return a null result if tx.to is not found on the network. I'd suggest instead of checking if (verifyAccount !== null && verifyAccount.receiver_sig_required === true) for the sig required, we can pull the verifyAccount !== null out and add a new block to check the validity of the verifyAccount. If it's null then throw RESOURCE_NOT_FOUND like how we did with the tx.from.

quiet-node · 2024-12-11T15:29:42Z

packages/server/tests/acceptance/rpc_batch1.spec.ts

+        it('should fail "eth_sendRawTransaction" if receiver\'s account has receiver_sig_required enabled', async function () {
+          const newPrivateKey = PrivateKey.generateED25519();
+          const newAccount = await new AccountCreateTransaction()
+            .setKey(newPrivateKey.publicKey)
+            .setInitialBalance(100)
+            .setReceiverSignatureRequired(true)
+            .freezeWith(servicesNode.client)
+            .sign(newPrivateKey);
+
+          const transaction = await newAccount.execute(servicesNode.client);
+          const receipt = await transaction.getReceipt(servicesNode.client);
+
+          if (!receipt.accountId) {
+            throw new Error('Failed to create new account - accountId is null');
+          }
+
+          const toAddress = Utils.idToEvmAddress(receipt.accountId.toString());
+          const tx = {
+            nonce: await accounts[0].wallet.getNonce(),
+            chainId: CHAIN_ID,
+            to: toAddress,
+            from: accounts[0].address,
+            value: '0x2E90EDD000',
+            gasLimit: defaultGasLimit,
+            accessList: [],
+            maxPriorityFeePerGas: defaultGasPrice,
+            maxFeePerGas: defaultGasPrice,
+          };
+
+          const signedTx = await accounts[0].wallet.signTransaction(tx);
+          await new Promise((r) => setTimeout(r, 3000));
+
+          const error = predefined.RECEIVER_SIGNATURE_REQUIRED;
+
+          await Assertions.assertPredefinedRpcError(error, sendRawTransaction, false, relay, [
+            signedTx,
+            requestDetails,
+          ]);
+        });


Great test! Let's add another test to ensure it works fine when receiver.to has receiver_sig_required disabled.

Additionally, if we decide to throw a RESOURCE_NOT_FOUND error for tx.to, we should include another test that uses an address not present in the system to verify that it blocks the request.

nadezhdapopovaa requested review from AlfredoG87, lukelee-sl and a team as code owners December 5, 2024 12:30

nadezhdapopovaa linked an issue Dec 5, 2024 that may be closed by this pull request

Add prechecks to the sendRawTransaction method to verify the validity of the receiver's account address. #3286

Open

nadezhdapopovaa self-assigned this Dec 5, 2024

nadezhdapopovaa added the enhancement New feature or request label Dec 5, 2024

nadezhdapopovaa added this to the 0.62.0 milestone Dec 5, 2024

nadezhdapopovaa force-pushed the 3286-add-prechecks-to-the-sendrawtransaction-method-to-verify-the-validity-of-the-receivers-account-address branch from 589773c to 92c28db Compare December 5, 2024 14:56

quiet-node requested changes Dec 5, 2024

View reviewed changes

quiet-node reviewed Dec 5, 2024

View reviewed changes

nadezhdapopovaa force-pushed the 3286-add-prechecks-to-the-sendrawtransaction-method-to-verify-the-validity-of-the-receivers-account-address branch 2 times, most recently from 66403c6 to 7931099 Compare December 11, 2024 14:39

nadezhdapopovaa added 4 commits December 11, 2024 16:52

chore: add precheck for receivers account in sendRawTransactionCheck

a8fdd86

Signed-off-by: Nadezhda Popova <[email protected]>

fixup! chore: add precheck for receivers account in sendRawTransactio…

a892247

…nCheck Signed-off-by: Nadezhda Popova <[email protected]>

fixup! fixup! chore: add precheck for receivers account in sendRawTra…

4604f1f

…nsactionCheck Signed-off-by: Nadezhda Popova <[email protected]>

test: add test for receivers account precheck

3b85b1a

Signed-off-by: Nadezhda Popova <[email protected]>

nadezhdapopovaa force-pushed the 3286-add-prechecks-to-the-sendrawtransaction-method-to-verify-the-validity-of-the-receivers-account-address branch from 7931099 to 3b85b1a Compare December 11, 2024 14:53

quiet-node requested changes Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add precheck for receivers account in sendRawTransactionCheck #3310

chore: add precheck for receivers account in sendRawTransactionCheck #3310

nadezhdapopovaa commented Dec 5, 2024

github-actions bot commented Dec 5, 2024 •

edited

Loading

codecov bot commented Dec 5, 2024

quiet-node left a comment

quiet-node Dec 5, 2024

quiet-node Dec 5, 2024

quiet-node Dec 5, 2024 •

edited

Loading

Nana-EC Dec 11, 2024

quiet-node Dec 5, 2024

quiet-node Dec 5, 2024

quiet-node Dec 5, 2024

Nana-EC Dec 11, 2024

quiet-node Dec 11, 2024

quiet-node Dec 11, 2024

sonarcloud bot commented Dec 11, 2024

quiet-node left a comment

quiet-node Dec 11, 2024

quiet-node Dec 11, 2024

chore: add precheck for receivers account in sendRawTransactionCheck #3310

Are you sure you want to change the base?

chore: add precheck for receivers account in sendRawTransactionCheck #3310

Conversation

nadezhdapopovaa commented Dec 5, 2024

github-actions bot commented Dec 5, 2024 • edited Loading

Test Results

codecov bot commented Dec 5, 2024

Codecov Report

quiet-node left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quiet-node Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Dec 11, 2024

Quality Gate passed

quiet-node left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 5, 2024 •

edited

Loading

quiet-node Dec 5, 2024 •

edited

Loading